drivers: entropy: stm32: make get_entropy_isr re-entrant
#91593
+84
−25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current `get_entropy_isr` implementation is not re-entrant; in fact, it will always deadlock when a re-entrant call is performed, because the logic to determine whether a call should disable the RNG or not after completion is inverted (the RNG is "acquired" when its interrupt is DISABLED, not when it is ENABLED), so re-entrant calls will always disable the RNG before return. Depending on where the original/pre-empted call was when it was interrupted, this can lead to the function polling the (now disabled) RNG for entropy that will never arrive. Inverting the logic seems like a fix, but is actually incomplete as there are situations where this information is not sufficient alone to determine the proper action to take. Implement proper logic to determine whether a call is re-entrant, using the status of the RNG peripheral clock, the RNG status register and the HSEM on series where it is relevant. Signed-off-by: Mathieu Choplain <mathieu.choplain@st.com>
github-actions
bot
requested review from
ceolin,
djiatsaf-st,
erwango,
etienne-lms,
FRASTM,
gautierg-st,
GeorgeCGV,
marwaiehm-st and
tomi-font
|
etienne-lms
reviewed
Jun 27, 2025
| * but implemented in v1.1.0 - this wrapper can be removed | ||
| * when the STM32CubeWB0 is updated in hal_stm32. | ||
| */ | ||
| return (READ_BIT(RNGx->CR, RNG_CR_DISABLE) == LL_RNG_CR_DISABLE_0); |
There was a problem hiding this comment.
Nitpicking: mimic expect LL implementation and prevent bool/uint32_t implicit cast?
return READ_BIT(RNGx->CR, RNG_CR_DISABLE) != (RNG_CR_DISABLE)) ? 1UL : 0UL;| { | ||
| #if defined(CONFIG_SOC_STM32WB09XX) | ||
| /** | ||
| * LL_RNG_IsEnabled() is missing from STM32CubeWB0 v1.0.0 |
There was a problem hiding this comment.
Nitpciking also:
Suggested change
| * LL_RNG_IsEnabled() is missing from STM32CubeWB0 v1.0.0 | |
| * LL_RNG_IsEnabled() is missing from STM32CubeWB0 v1.0.0 for WB09 |
| /** | ||
| * z_stm32_hsem_is_owned() evaluates to false on single-CPU SoCs, | ||
| * but we always own RNG on these SoCs since there's only one CPU. | ||
| */ |
There was a problem hiding this comment.
entropy_stm32.c is the sole caller of z_stm32_hsem_is_owned(). For consistency I think this SoC helper function should return true if there is no other possible owner, that is when not WBX, MPx or H7 dual core.
| */ | ||
| if (z_stm32_hsem_is_owned(CFG_HW_RNG_SEMID)) { | ||
| rng_already_acquired = true; | ||
| } | ||
| acquire_rng(); |
There was a problem hiding this comment.
Is it ok to acquire the RNG (possibly polling on a HSEM) with interrupts locked?
|
Converting to draft and moving to 4.3.0 as this will require rework |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current
get_entropy_isrimplementation is not re-entrant; in fact, it will always deadlock when a re-entrant call is performed, because the logic to determine whether a call should disable the RNG or not after completion is inverted (the RNG is "acquired" when its interrupt is DISABLED, not when it is ENABLED) ==> re-entrant calls will always disable the RNG before return. Depending on where the original/pre-empted call was when it was interrupted, this can lead to the function polling the (now disabled) RNG for entropy that will never arrive. Inverting the logic seems like a fix, but is actually incomplete as there are situations where this information is not sufficient alone to determine the proper action to take.Implement proper logic to determine whether a call is re-entrant, using the status of the RNG peripheral clock, the RNG status register and the HSEM on series where it is relevant.